cargo-auditable-cargo-wrapper: don't wrap if cargo-auditable is meta.broken #250615

ghost · 2023-08-21T20:45:49Z

Recent changes to cargo-auditable-cargo-wrapper and librsvg caused it to ignore the user's decision to opt out of cargo-audit functionality, partially because librsvg does not use buildRustPackage.

This commit restores the single-point-of-opt-out from this mis-named functionality: cargo-audit.meta.broken.

figsoda · 2023-08-21T20:55:28Z

cargo's auditable option is only about whether cargo itself is auditable, not whether it produces auditable binaries. The option is exposed to boostrap cargo-auditable.

ghost · 2023-08-23T04:52:42Z

cargo's auditable option is only about whether cargo itself is auditable

So where is the master switch to disable "audit" everywhere, including packages like librsvg that don't use buildRustPackage?

DieracDelta · 2023-08-23T12:23:54Z

This change appears to break overlays that override cargo (such as fenix).

figsoda · 2023-08-23T19:57:30Z

cargo's auditable option is only about whether cargo itself is auditable

So where is the master switch to disable "audit" everywhere, including packages like librsvg that don't use buildRustPackage?

You can overlay cargo-auditable-cargo-wrapper to just cargo, and maybe also buildRustPackage

ghost · 2023-08-23T20:02:59Z

This change appears to break overlays that override cargo (such as fenix).

Yeah, well, cargo-auditable-cargo-wrapper broke overlays that disable auditable in non-buildRustPackage packages (like librsvg), which I think is more obnoxious. Also, this doesn't break overlays that pass through the auditable flag; I'd say that's now part of what you need to do in order to override cargo.

You can overlay cargo-auditable-cargo-wrapper to just cargo, and maybe also buildRustPackage

Until recently you only had to do one of those two things.

In the future will people have to do three things?

There needs to be one single place where users can opt out of this, instead of having to constantly track the development of a feature simply to not use it.

I welcome suggestions on alternative locations for that single point-of-opt-out.

figsoda · 2023-08-23T20:26:13Z

Can I ask why you would need to globally turn off cargo-auditable?

ghost · 2023-08-23T20:29:13Z

Can I ask why you would need to globally turn of cargo-auditable?

Because I don't want to use it.

Also, like so many things related to cargo, it duplicates functionality that nix already provides. And does so quite poorly.

figsoda · 2023-08-23T20:36:40Z

Are there drawback to using cargo-auditable?

ghost · 2023-08-23T20:42:25Z

Are there drawback to using cargo-auditable?

Please don't derail the discussion.

figsoda · 2023-08-23T20:44:18Z

Are there drawback to using cargo-auditable?

Please don't derail the discussion.

I'm just asking because I would prefer to fix the issue you are experiencing instead of having to create a workaround, if that's possible.

ghost · 2023-08-23T20:46:10Z

I'm just asking because I would prefer to fix the issue you are experiencing

The issue I am experiencing is that I do not wish to use cargo-audit, yet it keeps re-enabling itself when I rebase my development branch.

This PR fixes the issue I am experiencing. You can fix the issue by merging it.

figsoda

I would appreciate it if you weren't being passive-aggressive in the replies.

Anyway, I don't think we should introduce a passthru for this, instead we can just mark cargo-auditable as broken in the overlay.

With this patch

--- a/pkgs/development/compilers/rust/1_71.nix
+++ b/pkgs/development/compilers/rust/1_71.nix
@@ -52,7 +52,7 @@ import ./default.nix {
     mips64el-unknown-linux-gnuabi64 = "de5fd0b249fbb95b9b67928ba08d7ec49f18f0ae25cbe1b0ede3c02390d7b93a";
   };
 
-  selectRustPackage = pkgs: pkgs.rust_1_71;
+  selectRustPackage = pkgs: pkgs.rust;
 
   rustcPatches = [ ];
 }
--- a/pkgs/development/compilers/rust/cargo-auditable-cargo-wrapper.nix
+++ b/pkgs/development/compilers/rust/cargo-auditable-cargo-wrapper.nix
@@ -1,13 +1,16 @@
 { lib, runCommand, makeBinaryWrapper, cargo, cargo-auditable }:
 
-runCommand "auditable-${cargo.name}" {
-  nativeBuildInputs = [ makeBinaryWrapper ];
-  meta = cargo-auditable.meta // {
-    mainProgram = "cargo";
-  };
-} ''
-  mkdir -p $out/bin
-  makeWrapper ${cargo}/bin/cargo $out/bin/cargo \
-    --set CARGO_AUDITABLE_IGNORE_UNSUPPORTED 1 \
-    --prefix PATH : ${lib.makeBinPath [ cargo cargo-auditable ]}
-''
+if cargo-auditable.meta.broken then
+  cargo
+else
+  runCommand "auditable-${cargo.name}" {
+    nativeBuildInputs = [ makeBinaryWrapper ];
+    meta = cargo-auditable.meta // {
+      mainProgram = "cargo";
+    };
+  } ''
+    mkdir -p $out/bin
+    makeWrapper ${cargo}/bin/cargo $out/bin/cargo \
+      --set CARGO_AUDITABLE_IGNORE_UNSUPPORTED 1 \
+      --prefix PATH : ${lib.makeBinPath [ cargo cargo-auditable ]}
+  ''

The following overlay works for me

final: prev: {
  rustPackages = prev.rustPackages.overrideScope (_: prevRust: {
    cargo-auditable = prevRust.cargo-auditable.overrideAttrs (old: {
      meta = old.meta // {
        broken = true;
      };
    });
  });
  rust = prev.rust // {
    packages = prev.rust.packages // {
      stable = final.rustPackages;
    };
  };
}

It is a little more code than I would like, but one could possibly deduplicate the top-level rust attributes to simplify the overlay. Though that would probably go into a separate PR.

figsoda

For the commit message, I don't think cargo-auditable: respect user's choices is descriptive enough. For my suggested solution, it should probably be something like cargo-auditable-cargo-wrapper: don't wrap if cargo-auditable is broken.

And for the rest of the commit message, it should be cargo-auditable instead of cargo-audit, and I assume you mean buildRustPackage instead of buildRustCrate?

…broken Recent changes to `cargo-auditable-cargo-wrapper` and `librsvg` caused it to ignore the user's decision to opt out of `cargo-audit` functionality, partially because `librsvg` does not use `buildRustPackage`. This commit restores the single-point-of-opt-out from this mis-named functionality: `cargo-audit.meta.broken`.

ghost · 2023-08-24T03:56:50Z

I would appreciate it if you weren't being passive-aggressive in the replies.

I don't see anything aggressive about my replies.

Nixpkgs is policy-free and I want to keep it that way. My own policy choices are not relevant to that goal. Every time somebody tries to impose their own policy choices on nixpkgs, the debate takes a predictable path: they try to steer the conversation towards my policy choices, and explain why their policy choices are better. It's tedious and distracting, so I refuse to be drawn into it. Aggression has nothing to do with it.

Anyway, I don't think we should introduce a passthru for this, instead we can just mark cargo-auditable as broken

Yes, I've been doing that, which is how I discovered that cargo-auditable-cargo-wrapper ignores cargo-auditable.meta.broken.

With this patch

(which causes cargo-auditable-cargo-wrapper to check meta.broken)

The following overlay works for me

I'm not excited about further-overloading meta.broken (which is already used for too many things), but it's a compromise I'm okay with if it gets you on board.

I assume you mean buildRustPackage instead of buildRustCrate?

Yes, thanks for noticing that.

ghost · 2023-08-24T04:00:26Z

Rebuilding to verify this; will undraftify when built.

tjni · 2023-08-24T08:43:15Z

I do think we need better ways to control global policy decisions, but I don't have a design in mind. This change is great, it's the right thing to do to fall back to cargo in situations when cargo-auditable is broken, and if it works to solve the policy problem (with a little bit creativity and shielding your eyes), then even better.

RaitoBezarius · 2023-08-24T09:06:47Z

Are there drawback to using cargo-auditable?

I would say that drawbacks are listed here: rust-lang/rfcs#2801 — either case, I find cargo-auditable to be extremely cheap and would pursue enabling it by default on a large scale beyond Nixpkgs, obviously, I also agree that it's important to let anyone override this default and opt out as they deem it fit, including, beyond Nixpkgs.

It will make things easier when this feature lands upstream hopefully.

figsoda

I wasn't able to make it work without this patch:

--- a/pkgs/development/compilers/rust/1_71.nix
+++ b/pkgs/development/compilers/rust/1_71.nix
@@ -52,7 +52,7 @@ import ./default.nix {
     mips64el-unknown-linux-gnuabi64 = "de5fd0b249fbb95b9b67928ba08d7ec49f18f0ae25cbe1b0ede3c02390d7b93a";
   };
 
-  selectRustPackage = pkgs: pkgs.rust_1_71;
+  selectRustPackage = pkgs: pkgs.rust;
 
   rustcPatches = [ ];
 }

but lgtm if you managed to make this work

one nitpick in the commit message: cargo-audit -> cargo-auditable, though it can be edited with a squash merge so we don't have to wait for ofborg again

tjni · 2023-08-26T20:29:20Z

@figsoda did something break for you without that patch? I don't see a clear connection from the code.

figsoda · 2023-08-26T20:32:39Z

@figsoda did something break for you without that patch? I don't see a clear connection from the code.

This overlay wasn't working correctly for me without that patch, you would probably need to overlay rust_1_71 instead of rust for it to work, which wouldn't be very ideal

final: prev: {
  rustPackages = prev.rustPackages.overrideScope (_: prevRust: {
    cargo-auditable = prevRust.cargo-auditable.overrideAttrs (old: {
      meta = old.meta // {
        broken = true;
      };
    });
  });
  rust = prev.rust // {
    packages = prev.rust.packages // {
      stable = final.rustPackages;
    };
  };
}

tjni · 2023-08-26T20:42:01Z

Oh, that makes sense now. I'm ok with this being hard to overlay since the scope is reduced to handling builds gracefully on systems that cargo-auditable doesn't support. There's still room for future work targeted at making usage of cargo-auditable a configurable option.

@amjoseph-nixpkgs can you address the typo that was pointed out in the commit message (i.e. cargo-audit -> cargo-auditable)? I think we can merge then.

figsoda · 2023-08-26T21:03:53Z

@amjoseph-nixpkgs can you address the typo that was pointed out in the commit message (i.e. cargo-audit -> cargo-auditable)? I think we can merge then.

If you want, you can edit the commit message with a squash merge. Not sure if this change alone fixes @amjoseph-nixpkgs's issues, but this PR lgtm.

tjni · 2023-08-26T21:07:15Z

Cool, it worked!

github-actions bot added the 6.topic: rust label Aug 21, 2023

ghost marked this pull request as ready for review August 21, 2023 20:46

ghost requested review from Mic92, zowoq, winterqt and figsoda as code owners August 21, 2023 20:46

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Aug 23, 2023

DieracDelta mentioned this pull request Aug 23, 2023

fix: nix on ARM EspressoSystems/HotShot#1607

Merged

ghost marked this pull request as draft August 23, 2023 20:07

ghost changed the base branch from master to staging August 23, 2023 20:08

ghost marked this pull request as ready for review August 23, 2023 20:09

ghost changed the title ~~cargo-auditable-cargo-wrapper: respect user's auditable=false choices~~ cargo-auditable: respect user's choices Aug 23, 2023

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 and removed 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ labels Aug 23, 2023

ofborg bot removed 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Aug 23, 2023

figsoda reviewed Aug 23, 2023

View reviewed changes

ghost marked this pull request as draft August 24, 2023 03:56

ghost changed the title ~~cargo-auditable: respect user's choices~~ cargo-auditable-cargo-wrapper: don't wrap if cargo-auditable is meta.broken Aug 24, 2023

ghost requested a review from figsoda August 24, 2023 03:57

ghost marked this pull request as ready for review August 24, 2023 05:31

tjni approved these changes Aug 24, 2023

View reviewed changes

figsoda approved these changes Aug 24, 2023

View reviewed changes

tjni merged commit bac515f into NixOS:staging Aug 26, 2023
10 checks passed

ghost deleted the pr/cargo/respect-user-choices branch January 23, 2024 06:47

Artturin mentioned this pull request Jan 29, 2024

List of @amjoseph-nixpkgs Pull requests before account removal #284796

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo-auditable-cargo-wrapper: don't wrap if cargo-auditable is meta.broken #250615

cargo-auditable-cargo-wrapper: don't wrap if cargo-auditable is meta.broken #250615

ghost commented Aug 21, 2023 •

edited by ghost

Loading

figsoda commented Aug 21, 2023

ghost commented Aug 23, 2023

DieracDelta commented Aug 23, 2023 •

edited

Loading

figsoda commented Aug 23, 2023

ghost commented Aug 23, 2023 •

edited by ghost

Loading

figsoda commented Aug 23, 2023 •

edited

Loading

ghost commented Aug 23, 2023 •

edited by ghost

Loading

figsoda commented Aug 23, 2023

ghost commented Aug 23, 2023

figsoda commented Aug 23, 2023

ghost commented Aug 23, 2023

figsoda left a comment

figsoda left a comment •

edited

Loading

ghost commented Aug 24, 2023

ghost commented Aug 24, 2023

tjni commented Aug 24, 2023

RaitoBezarius commented Aug 24, 2023

figsoda left a comment •

edited

Loading

tjni commented Aug 26, 2023

figsoda commented Aug 26, 2023

tjni commented Aug 26, 2023 •

edited

Loading

figsoda commented Aug 26, 2023

tjni commented Aug 26, 2023

cargo-auditable-cargo-wrapper: don't wrap if cargo-auditable is meta.broken #250615

cargo-auditable-cargo-wrapper: don't wrap if cargo-auditable is meta.broken #250615

Conversation

ghost commented Aug 21, 2023 • edited by ghost Loading

figsoda commented Aug 21, 2023

ghost commented Aug 23, 2023

DieracDelta commented Aug 23, 2023 • edited Loading

figsoda commented Aug 23, 2023

ghost commented Aug 23, 2023 • edited by ghost Loading

figsoda commented Aug 23, 2023 • edited Loading

ghost commented Aug 23, 2023 • edited by ghost Loading

figsoda commented Aug 23, 2023

ghost commented Aug 23, 2023

figsoda commented Aug 23, 2023

ghost commented Aug 23, 2023

figsoda left a comment

Choose a reason for hiding this comment

figsoda left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Aug 24, 2023

ghost commented Aug 24, 2023

tjni commented Aug 24, 2023

RaitoBezarius commented Aug 24, 2023

figsoda left a comment • edited Loading

Choose a reason for hiding this comment

tjni commented Aug 26, 2023

figsoda commented Aug 26, 2023

tjni commented Aug 26, 2023 • edited Loading

figsoda commented Aug 26, 2023

tjni commented Aug 26, 2023

ghost commented Aug 21, 2023 •

edited by ghost

Loading

DieracDelta commented Aug 23, 2023 •

edited

Loading

ghost commented Aug 23, 2023 •

edited by ghost

Loading

figsoda commented Aug 23, 2023 •

edited

Loading

ghost commented Aug 23, 2023 •

edited by ghost

Loading

figsoda left a comment •

edited

Loading

figsoda left a comment •

edited

Loading

tjni commented Aug 26, 2023 •

edited

Loading